Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Do not overwrite openai.api_version. #4234

Closed
wants to merge 3 commits into from
Closed

Do not overwrite openai.api_version. #4234

wants to merge 3 commits into from

Conversation

zijie0
Copy link

@zijie0 zijie0 commented May 6, 2023 •
edited

I found an issue that the value of openai.api_version was modified after creating the embedding model:

In [1]: import os
   ...: import openai
   ...: from langchain.chat_models import AzureChatOpenAI
   ...: from langchain.schema import HumanMessage
   ...: from langchain.embeddings import OpenAIEmbeddings
   ...:
   ...: os.environ["OPENAI_API_TYPE"] = "azure"
   ...: os.environ["OPENAI_API_VERSION"] = "2023-03-15-preview"
   ...: os.environ["OPENAI_API_BASE"] = "https://dummy.openai.azure.com/"
   ...: os.environ["OPENAI_API_KEY"] = "dummy_key"
   ...:
   ...: openai.api_type = os.getenv("OPENAI_API_TYPE")
   ...: openai.api_version = os.getenv("OPENAI_API_VERSION")
   ...: openai.api_base = os.getenv("OPENAI_API_BASE")
   ...: openai.api_key = os.getenv("OPENAI_API_KEY")

In [2]: print(openai.api_version)
2023-03-15-preview

In [3]: emb_model = OpenAIEmbeddings()

In [4]: print(openai.api_version)
2022-12-01

This will lead to many weird problems, such as environment variables and named parameter will NOT work in subsequent use of other Azure OpenAI models and lead to "Resource not found" errors (gpt-3.5 and gpt-4).

The fix is simple. But it may cause some regression error if user didn't set OPENAI_API_VERSION in environment variables nor pass it as a named parameter.

pranjaldoshi96
pranjaldoshi96 reviewed May 7, 2023
View reviewed changes
langchain/embeddings/openai.py
@@ -106,7 +106,7 @@ class OpenAIEmbeddings(BaseModel, Embeddings):
client: Any #: :meta private:
model: str = "text-embedding-ada-002"
deployment: str = model # to support Azure OpenAI Service custom deployment names
openai_api_version: str = "2022-12-01"
openai_api_version: str = ""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why make default value "" you can have a default value at https://github.com/hwchase17/langchain/pull/4234/files#diff-d7a6d6752d04755e0881f71a73e707932889769c486afac9ede77b92ea125cbbR147

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pranjaldoshi96 I made the default value "" according to other azure model implementations: https://github.com/zijie0/langchain/blob/96f4467b3c26eceae28cedd6a5180adbcda6cd9e/langchain/chat_models/azure_openai.py#L45

By making the default "", we are forcing user to define this variable either in env or in named parameter.

You remind me that this parameter should be optional with default OpenAI embeddings. So I also added openai_api_type check in validation.

@zijie0
Copy link
Author

zijie0 commented May 8, 2023

Some related issue: #3510 #3444 #3433 .

@zijie0 zijie0 requested a review from pranjaldoshi96 May 9, 2023 04:13
@zijie0
Copy link
Author

zijie0 commented May 11, 2023

Hey @hwchase17 , could anyone take a look at this? I think it might solve many potential issues when using Azure OpenAI.

@dev2049 dev2049 self-assigned this May 11, 2023
@hwchase17
Copy link
Contributor

@zijie0 sorry for delay! how does #4687 look?

@zijie0
Copy link
Author

zijie0 commented May 15, 2023

@hwchase17 LGTM! I think it should solve my problem.

@zijie0 zijie0 closed this May 15, 2023
This was referenced May 15, 2023
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pranjaldoshi96 pranjaldoshi96 Awaiting requested review from pranjaldoshi96

Assignees

@dev2049 dev2049

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@zijie0 @hwchase17 @pranjaldoshi96 @dev2049